Skip to content

Save and restore playback parameters into/from preferences#3288

Merged
TobiGr merged 1 commit intoTeamNewPipe:devfrom
XiangRongLin:save-playback
Apr 8, 2020
Merged

Save and restore playback parameters into/from preferences#3288
TobiGr merged 1 commit intoTeamNewPipe:devfrom
XiangRongLin:save-playback

Conversation

@XiangRongLin
Copy link
Collaborator

@XiangRongLin XiangRongLin commented Mar 28, 2020 •
edited
Loading

What is it?

  • Feature

Long description of the changes in your PR

  • Playback parameters are speed, pitch and skip silence
  • Save playback parameters into shared preferences whenever they are set
  • Retrieve playback parameters from shared preferences when starting any player
  • Remove playback parameters from being passed from one player to another through intents. Instead they can be retrieved from the shared preferences.

Fixes the following issue(s)

Testing apk

app-debug.zip

Agreement

Note

I noticed that changing the playback parameters while the video is paused gets ignored. At first I thought i broke something, until I noticed that it was like this before too.

@wb9688 wb9688 mentioned this pull request Mar 28, 2020
Closed
3 tasks
wb9688
wb9688 previously approved these changes Mar 28, 2020
View reviewed changes
Copy link
Member

@wb9688 wb9688 left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me, though I didn't look in detail and I don't think this should be merged for v0.19.0.

@XiangRongLin
Copy link
Collaborator Author

XiangRongLin commented Mar 28, 2020

After seeing the other PR for the same issue I realized that the formatting/parsing of the float when saving/retrieving is completely unnecessary.

I'll remove that tomorrow.

@XiangRongLin
Copy link
Collaborator Author

XiangRongLin commented Mar 29, 2020

@wb9688
I just removed the unnecessary formatting/parsing.

New apk: app-debug.zip
Data needs to be cleared, if you are just installing over my previous apk

wb9688
wb9688 requested changes Mar 29, 2020
View reviewed changes
Copy link
Member

@wb9688 wb9688 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other than these small things, this PR is good.

wb9688
wb9688 reviewed Mar 29, 2020
View reviewed changes
wb9688
wb9688 previously approved these changes Mar 29, 2020
View reviewed changes
@Stypox
Copy link
Member

Stypox commented Mar 30, 2020

I'm not completely sure if this is ok because in the background player there is a completely different probability of speeding up playback (I do it once in a blue moon) from other players (where I speed up things 95% of the times). So in the end I would have to tweak parameters anyway every time I switch players.

@Stypox
Copy link
Member

Stypox commented Mar 30, 2020 •
edited
Loading

Though this is surely better than the current behaviour, so if no one comes up with a better solution this is the way to go.

Maybe making it so that:

  1. there are different playback parameters for every player
  2. the only way to change the parameters for a player is from its interface (like now, but this assertion is needed for 4.)
  3. when a player is accessed directly, its parameters are loaded
  4. when a player is switched to from another player, the parameters are carried, but are not saved unless the user manually changes something

@XiangRongLin XiangRongLin mentioned this pull request Mar 30, 2020
Closed
@TobiGr
Copy link
Contributor

TobiGr commented Mar 30, 2020

@Stypox Our three players will be unified into one in #2907, so there won't be three players any more soon.

@XiangRongLin
Save and restore playback parameters into/from preferences
3855e48 
Playback parameters are speed, pitch and skip silence.
Remove parameters being passed on as intent to the player, since the parameters can be restored from the preferences instead.

# Conflicts:
#	app/src/main/java/org/schabi/newpipe/player/BasePlayer.java
@TobiGr TobiGr added this to the 0.19.3 milestone Apr 6, 2020
wb9688
wb9688 approved these changes Apr 8, 2020
View reviewed changes
Copy link
Member

@wb9688 wb9688 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code itself is OK. @Stypox' concerns are valid though, however I think we should only look at that once we have the unified player and just merge this now.

@TobiGr TobiGr merged commit 65cd975 into TeamNewPipe:dev Apr 8, 2020
@XiangRongLin XiangRongLin deleted the save-playback branch April 9, 2020 14:13
@Stypox Stypox mentioned this pull request Apr 10, 2020
Closed
This was referenced Apr 24, 2020
Merged
Closed
@XiangRongLin XiangRongLin mentioned this pull request Dec 31, 2020
Open
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@wb9688 wb9688 wb9688 approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

0.19.3

Development

Successfully merging this pull request may close these issues.

4 participants

@XiangRongLin @Stypox @TobiGr @wb9688